kuberuntime: check the value of RunAsNonRoot when verifying #47009
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/cc @feiskyer |
k8s-github-robot
added
approved
|
@liggitt this should fix the kubelet issue we discussed earlier. |
|
LGTM, would like a second from @pweil- |
The verification function is fixed to check the value of RunAsNonRoot, not just the existence of it. Also adds unit tests to verify the correct behavior.
|
I pushed again just to remove the two redundant lines (48-49 in security_context_test.go) from the original test. Nothing else was changed. |
| @@ -72,7 +72,8 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po | |||
| // verifyRunAsNonRoot verifies RunAsNonRoot. | |||
| func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error { | |||
| effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container) | |||
| if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil { | |||
| // If the option is not set, or if running as root is allowed, return nil. | |||
| if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot { | |||
There was a problem hiding this comment.
This is a k8s api pod object. I don't think we have getters for them.
| @@ -244,7 +244,7 @@ func TestGenerateContainerConfig(t *testing.T) { | |||
| Command: []string{"testCommand"}, | |||
| WorkingDir: "testWorkingDir", | |||
| SecurityContext: &v1.SecurityContext{ | |||
| RunAsNonRoot: &RunAsNonRoot, | |||
| RunAsNonRoot: &runAsNonRootTrue, | |||
There was a problem hiding this comment.
What is the following expectedConfig used for?
There was a problem hiding this comment.
Probably can be removed. @feiskyer
But I don't want to turn this into a test cleanup PR. That can be done in a separate PR.
There was a problem hiding this comment.
Missing one assertion
assert.Equal(t, expectedConfig, containerConfig, "generate container config for kubelet runtime v1.")
| errStr: "container's runAsUser breaks non-root policy", | ||
| }, | ||
| { | ||
| desc: "Fail if image's user is root and RunAsNonRoot is true", |
There was a problem hiding this comment.
Add case Pass if image's user is non-root and RunAsNonRoot is true?
| @@ -46,59 +46,59 @@ func TestVerifyRunAsNonRoot(t *testing.T) { | |||
| } | |||
|
|
|||
| err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0)) | |||
| assert.NoError(t, err) | |||
| assert.NoError(t, err, "should pass if SecurityContext is not set") | |||
There was a problem hiding this comment.
Why not merge this case into the test table?
There was a problem hiding this comment.
I removed this before you left the comment ;)
| RunAsNonRoot: &runAsNonRootTrue, | ||
| RunAsUser: &rootUser, | ||
| }, | ||
| errStr: "container's runAsUser breaks non-root policy", |
There was a problem hiding this comment.
Hmm, not sure whether we want to check error string.
There was a problem hiding this comment.
I didn't want to change the original test behavior too much, but remove it any way. Done.
|
@k8s-bot pull-kubernetes-unit test this |
|
LGTM |
|
LGTM |
|
/assign |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, yujuhong Associated issue: 46996 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 46775, 47009) |
|
I haven't squashed yet... :-| |
sorry, didn't notice there is another commit 😢 |
The verification function is fixed to check the value of RunAsNonRoot,
not just the existence of it. Also adds unit tests to verify the correct
behavior.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #46996Release note: